Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Configure consumer tag names/prefixes #265

Merged
merged 3 commits into from
Nov 28, 2016
Merged

Configure consumer tag names/prefixes #265

merged 3 commits into from
Nov 28, 2016

Conversation

sldblog
Copy link
Contributor

@sldblog sldblog commented Nov 24, 2016

Allows us to configure the consumer names (tags) that are visible in RabbitMQ.

Tagging with specific names is useful for us because we have over 100 queues and lots of applications communicating. If we aim to be easily identifiable, we can do these things:

  • define a routing key convention that refers to the application publishing the message
  • define a queue name convention that identifies the consumers
  • identify consumers by tag names

Out of the above three we cannot do the last one. So this changeset enables that configuration :)

How-to

Hutch::Config.set(:consumer_tag_prefix, 'update-dispatcher')

Notes

The default tag name changes from bunny to hutch with this change.

Bonus

Explains on how to (still manually) update the generated configuration section in the readme.

Examples

Default (before the PR)

screen shot 2017-03-16 at 18 29 24

Default (after the PR)

screen shot 2016-11-25 at 15 27 21

Custom prefix

screen shot 2016-11-25 at 15 28 20

@michaelklishin
Copy link
Member

@olleolleolle thoughts? This looks reasonable to me.

Generate with

0. `yard doc lib/hutch/config.rb`
0. Copy the _Configuration_ section from `doc/Hutch/Config.html` here, with the anchor tags stripped.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

♥️ 💛 💚 💙 💜

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This is you coming back to improve the README: 🎩 !)

@olleolleolle
Copy link
Contributor

olleolleolle commented Nov 24, 2016

The feature is an improvement! So many thumbs: 👍


The failing test is about supporting MarchHare + Bunny: JRuby support.

LoadError: no such file to load -- bunny/channel

In order to get that working, perhaps we need to not use instance_double()? And not only require stuff that's only for 1 kind of Ruby.

Copy link
Contributor

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, MarchHare perhaps has an alternate way of expressing this?


def consumer_tag(queue)
prefix = Hutch::Config[:consumer_tag_prefix]
queue.channel.generate_consumer_tag(prefix)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@olleolleolle olleolleolle Nov 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, it does exist, just another way of expressing it: https://github.com/ruby-amqp/march_hare/pull/93/files

@sldblog
Copy link
Contributor Author

sldblog commented Nov 24, 2016

Oh I understand why instance_double wasn't used now. I didn't realise the underlying library is different on JRuby. Let me rework some of this. Thanks for the feedback 👍

@@ -45,9 +45,17 @@
end

it 'sets up a subscription' do
expect(queue).to receive(:subscribe).with(manual_ack: true)
expect(queue).to receive(:subscribe).with(consumer_tag: %r(^hutch\-.{36}$), manual_ack: true)
Copy link
Contributor Author

@sldblog sldblog Nov 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I need to double check AMQP/RabbitMQ spec for the maximum available length of the consumer tag name?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be a cool touch.

(Then we could figure out ways to validate configuration options. Example addition to the Config options' DSL: max_length: 34.)

Copy link
Contributor

@olleolleolle olleolleolle Nov 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It "depends", it seems: https://www.rabbitmq.com/amqp-0-9-1-reference.html#basic.consume.consumer-tag it is a shortstr, defined as "shortstr shortstr [short string]"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it's 255 bytes max: https://github.com/rabbitmq/rabbitmq-common/blob/rabbitmq_v3_6_0/codegen.py#L178-L179 ... I hope that's the same in 3.4.x as well :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, now we know that the longest prefix that can be used is: 256 - 1 - 36 = 219 bytes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally Hutch should loudly complain about prefixes that are longer, e.g. log an error and trim the prefix or fail to start.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opted for an "on-create" validation as validating the actual prefix length is weird (why 219 bytes?) and coupled to the implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the on-create validation sufficient/ok?

def unique_consumer_tag
prefix = Hutch::Config[:consumer_tag_prefix]
unique_part = SecureRandom.uuid
"#{prefix}-#{unique_part}"
Copy link
Contributor Author

@sldblog sldblog Nov 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up generating this myself. Quote from the commit:

Due to the interface differences between bunny (used for CRuby/MRI)
and march_hare (used for JRuby), it's not possible to consistently use
the generator function queue.channel.generate_consumer_tag for the
unique part of the consumer tags.

Instead we can generate the unique part ourselves without relying on
internals of these gems. Using SecureRandom.uuid seems like a good
idea.

Is this assumption valid? Is it okay to use SecureRandom here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is. SecureRandom is available on JRuby and produces values of a known fixed length (36 bytes).

@sldblog
Copy link
Contributor Author

sldblog commented Nov 24, 2016

⌛️ I also need to test these with my local RabbitMQ install to ensure it's not breaking on some other integration problems.

Allows us to configure the consumer names (tags) that are visible in
RabbitMQ.

Tagging with specific names is useful for us because we have over 100
queues and lots of applications communicating. If we aim to be easily
identifiable, we can do these things:

- define a routing key convention that refers to the application
  publishing the message
- define a queue name convention that identifies the consumers
- identify consumers by tag names

Out of the above three we cannot do the last one. So this changeset
enables that configuration :)

Note: this changes the default tag name from `bunny` to `hutch`.
Due to the interface differences between `bunny` (used for CRuby/MRI)
and `march_hare` (used for JRuby), it's not possible to consistently use
the generator function `queue.channel.generate_consumer_tag` for the
unique part of the consumer tags.

Instead we can generate the unique part ourselves without relying on
internals of these gems. Using `SecureRandom.uuid` seems like a good
idea.
[According to the AMQP specification][amqp-spec] the field is a
`shortstr`, which is [255 bytes long][codegen].

[amqp-spec]: https://www.rabbitmq.com/amqp-0-9-1-reference.html#basic.consume.consumer-tag
[codegen]: https://github.com/rabbitmq/rabbitmq-common/blob/rabbitmq_v3_6_0/codegen.py#L178-L179
@sldblog
Copy link
Contributor Author

sldblog commented Nov 25, 2016

I also need to test these with my local RabbitMQ install to ensure it's not breaking on some other integration problems.

💚

Default (now)

screen shot 2016-11-25 at 15 27 21

Custom prefix

screen shot 2016-11-25 at 15 28 20

Oh dear

screen shot 2016-11-25 at 15 29 54

@michaelklishin
Copy link
Member

michaelklishin commented Nov 28, 2016 via email

@sldblog
Copy link
Contributor Author

sldblog commented Nov 28, 2016

Consumer tags do not change and are rarely used in delivery handlers (probably never in Hutch).

@michaelklishin Does that mean it's okay to validate them when creating the subscriptions?

@michaelklishin
Copy link
Member

It does to me.

@sldblog
Copy link
Contributor Author

sldblog commented Nov 28, 2016

🎉 🙇

@michaelklishin michaelklishin self-assigned this Nov 28, 2016
@michaelklishin michaelklishin merged commit 7550018 into ruby-amqp:master Nov 28, 2016
@sldblog sldblog deleted the consumer-tag-prefixes branch November 28, 2016 11:41
@sldblog
Copy link
Contributor Author

sldblog commented Jan 31, 2017

@michaelklishin Would it be possible to release this as 0.24? If it's blocked on something, can I help unblocking it?

@michaelklishin
Copy link
Member

Yes.

@michaelklishin
Copy link
Member

0.24.0 is out.

@sldblog
Copy link
Contributor Author

sldblog commented Feb 2, 2017

❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants